Fix CI test to actually use the Postgres database#9954
Fix CI test to actually use the Postgres database#9954browniebroke wants to merge 13 commits intoencode:mainfrom
Conversation
…tent PKs instead of complex URL builders
There was a problem hiding this comment.
Pull request overview
This PR completes the CI/PostgreSQL test setup by ensuring tox passes DATABASE_URL through, adjusts pytest execution to avoid migration-related failures, and makes test behavior deterministic across database backends (notably PostgreSQL sequence behavior and queryset ordering).
Changes:
- Configure tox to pass
DATABASE_URLinto test environments so PostgreSQL-required tests are exercised. - Run pytest with
--no-migrationsand adjust tests/fixtures to avoid Postgres-specific failures (missing tables, sequence/PK differences). - Stabilize tests by enforcing deterministic queryset ordering and backend-aware validator expectations.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tox.ini |
Passes DATABASE_URL into tox environments to enable Postgres-backed test runs. |
pyproject.toml |
Adds --no-migrations to default pytest options to avoid migration/setup issues. |
tests/conftest.py |
Adds an autouse fixture to reset Postgres sequences between tests for predictable PKs. |
tests/test_validators.py |
Makes validator-count expectation backend-aware using connection.ops.integer_field_range. |
tests/test_relations_slug.py |
Adds order_by('pk') to avoid nondeterministic ordering across DBs. |
tests/test_relations_pk.py |
Adds order_by('pk') to avoid nondeterministic ordering across DBs. |
tests/test_relations_hyperlink.py |
Adds order_by('pk') to avoid nondeterministic ordering across DBs. |
tests/test_filters.py |
Increases a CharField(max_length=...) to avoid Postgres-enforced length failures. |
rest_framework/test.py |
Attempts to prevent DB connection closing during RequestsClient WSGI calls by mutating Django signal handlers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dependency_groups = | ||
| test | ||
| deps = | ||
| pass_env = |
There was a problem hiding this comment.
In [testenv:base], pass_env is currently set but left empty. In tox, setting pass_env = here overrides the inherited [testenv] value and will stop DATABASE_URL from being passed through for the base env, which undermines the goal of running Postgres-backed tests consistently. Either remove this pass_env override or include DATABASE_URL (and any other required vars) here as well.
| pass_env = |
| table_names = set(connection.introspection.table_names()) | ||
| models = [m for m in apps.get_models() if m._meta.db_table in table_names] | ||
| sql_list = connection.ops.sequence_reset_sql(no_style(), models) | ||
| if sql_list: | ||
| with connection.cursor() as cursor: | ||
| for sql in sql_list: | ||
| cursor.execute(sql) |
There was a problem hiding this comment.
This autouse fixture runs a full table introspection and sequence reset for every DB-using test on PostgreSQL (table_names() + apps.get_models() + executing reset SQL). That can add significant overhead to the test suite. Consider computing the relevant model list once per session (or caching sql_list) and only executing the reset SQL per test, or scoping the sequence reset to only the specific apps/tables that need stable PKs (or, ideally, updating the affected tests to avoid hard-coded PK assumptions).
There was a problem hiding this comment.
Each test use different models so that won't work in our test suite
| # Disconnect close_old_connections to prevent closing the | ||
| # database connection during tests, matching the behavior | ||
| # of Django's ClientHandler. | ||
| environ = self.get_environ(request) | ||
| wsgi_response = self.app(environ, start_response) | ||
| request_started.disconnect(close_old_connections) | ||
| request_finished.disconnect(close_old_connections) | ||
| try: | ||
| wsgi_response = self.app(environ, start_response) | ||
| finally: | ||
| request_started.connect(close_old_connections) | ||
| request_finished.connect(close_old_connections) |
There was a problem hiding this comment.
Temporarily disconnecting/reconnecting global request_started/request_finished signal receivers inside DjangoTestAdapter.send() is not thread-safe: concurrent requests in the same process could run while the handlers are disconnected, changing behavior outside this request. Also, if close_old_connections was connected with a dispatch_uid (or otherwise not matched by the disconnect() call), the connect() in finally can add a duplicate receiver. A safer approach is to avoid mutating global signals here (e.g., use Django’s ClientHandler for the WSGI call path, or otherwise ensure disconnect/connect is idempotent via the same dispatch_uid that Django uses).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 8b41cc5.
browniebroke
force-pushed
the
fix/database-url-ci
branch
from
a4d2f30 to
d8df0f1
Compare
Description
After merging #9949 I realised that the setup wasn't complete and Postgres tests are in fact skipped:
This is because we run tests with tox and it needs to be instructed to pass environment variables down with the
pass_envoption.Once this was set, the tests started failing with
psycopg.errors.UndefinedTable: relation "auth_user" does not exist, which I fixed by passing--no-migrationsto pytest.Then a number of tests started failing due to hardcoded IDs and ordering assumptions which were no longer true.